Report offending input in register error messages - #1095
Conversation
Adds the failing input to the 9 validation errors in base_register.py that previously stated only the rule, following the pattern from pasqal-io#1094. Existing message text is kept intact and details appended, so all match= assertions in tests/ continue to pass without modification. Also fixes a missing space in the label-length message, which rendered as 'does notmatch'. Partially addresses pasqal-io#1057.
|
Hi @a-corni ready for review whenever you have time. For the trap_ids bounds check I mentioned at the bottom — happy to fold a fix |
a-corni
left a comment
a-corni
left a comment
There was a problem hiding this comment.
Thanks @jaewonyun1234 !
I have some comments, notably on the use of sorted. You can modify some error messages if you want.
Regarding the missing check on _validate_layout. I think you are right, and I suggest we make a call to register_layout.define_register to check the inputs properly.
| raise ValueError( | ||
| "If specifying 'kwargs', they must only be 'layout' and " | ||
| "'trap_ids'." | ||
| f"'trap_ids'; got {sorted(kwargs)}." |
There was a problem hiding this comment.
I am not sure the sorted is needed here. Since the target of this message is a software developer, I would rather return the kwargs in the order the program submitted them.
| f"Label length ({len(labels)}) does not" | ||
| f"match number of coordinates ({len(coords_)})" | ||
| f"Label length ({len(labels)}) does not " | ||
| f"match number of coordinates ({len(coords_)})." |
There was a problem hiding this comment.
Maybe report the two offending inputs: "Got coords {coords} and labels {labels}.".
| raise ValueError( | ||
| "The RegisterLayout dimensionality is not the same as this " | ||
| "register's." | ||
| f"register's; layout is {register_layout.dimensionality}D " | ||
| f"and register is {self.dimensionality}D." |
There was a problem hiding this comment.
I think what matters is knowing the input layout
. The information you are bringing has value, you can modify the error raised:
"The RegisterLayout dimensionality ({register_layout.dimensionality}D) is not the same as this "
f"register's ({self.dimensionality}D); Got layout {register_layout} on register {self.register}."
| "The IDs list must be selected among the IDs of the register's" | ||
| " qubits." | ||
| " qubits; " | ||
| f"{sorted(set(id_list) - set(self.qubit_ids))} not in " |
There was a problem hiding this comment.
I don't think the sorted is necessary here, and I would suggest to take it out as it might take unnecessary time to raise this error.
| ) | ||
| if len(set(trap_ids)) != len(trap_ids): | ||
| raise ValueError("Every 'trap_id' must be a unique integer.") | ||
| repeated = sorted({t for t in trap_ids if trap_ids.count(t) > 1}) |
There was a problem hiding this comment.
I think it would be best to try to return the repeating elements in their order of appearance in the trap_ids. Therefore, I suggest to get rid of sorted, and use collections.Counter (offers a faster one-liner than your current solution)
| repeated = sorted({t for t in trap_ids if trap_ids.count(t) > 1}) | |
| repeated = [t for t, freq in Counter(trap_ids).items() if freq > 1] |
| "The amount of 'trap_ids' must be equal to the number of atoms" | ||
| " in the register." | ||
| f" in the register; got {len(trap_ids)} trap_ids " | ||
| f"for {len(self._ids)} atoms." |
There was a problem hiding this comment.
You can modify the returned message to show the length of trap_ids and self._ids, but what matters is returning the incorrect input:
f"The amount of 'trap_ids' {len(trap_ids)} is not equal to the number of atoms {len(self._ids)}. Got trap ids {trap_ids} for atoms {self._ids}."
| "The qubit ids linked to detuning weights have to be defined" | ||
| " in the register." | ||
| " in the register. Got " | ||
| f"{sorted(set(detuning_weights) - set(self.qubit_ids))}, " |
There was a problem hiding this comment.
Here as well, I don't think the sorted is necessary and suggest to take it of, for the sake of time.
| for reg_coord, trap_id in zip( | ||
| self._coords_arr.as_array(detach=True), trap_ids | ||
| ): |
There was a problem hiding this comment.
Indeed, it is missing a check that the provided trap_ids are a subset of register_layout.traps_dict(). You could do register_layout.define_register(*trap_ids, qubit_ids=list(self.qubits)) to get all the checks from RegisterLayout.defin_register. You can include this test here, and properly test it in the tests, I would be fine with this. Otherwise, we can create a separate issue and tackle it separately, later. Let me know what you prefer.
2 participants
Same as #1094, applied to
base_register.py.Nine messages updated:
__init__,_init_kwargs,find_indices,from_coordinates, four in_validate_layout, anddefine_detuning_map.Also fixed a missing space that printed as
Label length (3) does notmatch number of coordinates (2).I left
"Cannot create a Register with an empty qubit dictionary."alone. Thedictionary is empty, so there's nothing to print.
One thing I noticed:
trap_coords[trap_id]on line 229 isn't bounds checked, soa bad trap id gives a numpy error instead of a Pulser one.
RegisterLayout.define_registercatches this but theRegisterconstructorskips that check. I didn't fix it here since it changes behaviour, not wording.
Partially addresses #1057.